feat!: TranslationOptions builder-like struct with Python bindings #308
Conversation
kalzoo
force-pushed
the
306-missing-python-bindings-owned-type
branch
from
0f82f29 to
934b326
Compare
|
One very minor thing - Otherwise, this looks good! I like the flexibility of the generic on the Rust side, even if it doesn't realize itself on the Python side as easily. I think it is a win regardless, but just curious about this:
Does the generic really solve that problem? The fastest way for a user to get access to new fields in the prost generated Either way, happy with this, and thanks for taking on the side quest! |
Good spot, fixed - thank you. As far as the ability to upgrade, it would still be compatible if it were within the version bounds in Cargo.toml. It may be the case that the grpc package isn't so much out of date and needs the user to upgrade it, but that not all options have been copied over into the hand-written struct/builder yet. From past experience, I'd say that the builder will probably only get updated by request or when those features are recognized as missing, so days might pass between (user wanting to use a new feature) and (that feature is actually available). The |
kalzoo
changed the title
Breaking change: it modifies the signature of
qpu::translate.This is an alternative approach to #307 , following recommendations made by @MarquessV . It supersedes that MR if accepted (it's not a merge train). It partially decouples this library from the QCS API's generated gRPC code.
This is partial because it still allows the library consumer to set translation options using the gRPC generated structs. However, it now defines a
TranslationOptionsstruct of its own, and a conversion from that struct to the gRPC generated struct. This is a middle ground between the options of (a) entirely decoupling from the gRPC generated code or (b) only accepting the gRPC generated struct, asmaincurrently does.The benefit is flexibility. Consider the alternatives:
translatein this library were to only accept theTranslationOptionsgenerated from the protobuf schema (as it currently does onmain), then consumers would have to use that struct directly. It's built from a wire format and thus is not the most intuitive to understand and use. As it stands today, the struct is rather simple and so would not cause too much trouble; however,oneofs and optional fields can be challenging and verbose to manipulate, especially when nested.translateonly accepted the new, hand-written struct defined here, then it would be a frequent occurrence for users to be unable to access new features in the API because the struct is out of date.Instead of those,
translatenow takestranslation_options: Option<impl Into<grpc::TranslationOptions>>so the user can take the path that works best for them. They can provide the less-ergonomic but possibly more-complete grpc generated struct, or the locally-defined, hand-written options struct, or anything else that implementsIntofor the grpc struct.Two downsides/considerations: